Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix crash during the sync page loading in tor(guest) window #1704

Merged
merged 1 commit into from
Feb 18, 2019
Merged

Fix crash during the sync page loading in tor(guest) window #1704

merged 1 commit into from
Feb 18, 2019

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Feb 18, 2019 •
edited
Loading

Sync page should be disabled for now because BraveSyncService isn't
instantiated for OTR profile.
Instead, sync is disabled page is loaded.

Fix brave/brave-browser#3003

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

Please refer STR in brave/brave-browser#3003

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@simonhong simonhong added this to the 0.63.x - Nightly milestone Feb 18, 2019
@simonhong simonhong self-assigned this Feb 18, 2019
@simonhong
Fix crash during the sync page loading in tor(guest) window
55eb56a 
Sync page should be disabled for now because BraveSyncService isn't
instantiated for OTR profile.
Instead, sync is disabled page is loaded.
@simonhong simonhong force-pushed the fix_crash_loading_sync_page_in_tor_widow branch from 26d2cd2 to 55eb56a Compare February 18, 2019 05:08
AlexeyBarabash
AlexeyBarabash reviewed Feb 18, 2019
View reviewed changes
components/brave_sync/brave_sync_service.cc
return false;
else
return true;
return !command_line.HasSwitch(switches::kDisableBraveSync);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

AlexeyBarabash
AlexeyBarabash approved these changes Feb 18, 2019
View reviewed changes
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for me.
I could not open brave://settings/ in either tor or guest window (STR 3), because it redirected to the tab in main profile window, looks like this was done in some other PR.

@simonhong
Copy link
Member Author

simonhong commented Feb 18, 2019 •
edited
Loading

@AlexeyBarabash Redirecting work was done by #1295.
Before that, only chrome://settings was redirected. brave://settings wasn't redirected. Also sync and rewards url are also handled in that PR.

However, it only works on private window(non tor or guest. Typing on brave://settings with tor window redirect to normal window? It's strange...

@AlexeyBarabash
Copy link
Contributor

AlexeyBarabash commented Feb 18, 2019 •
edited
Loading

@simonhong

Typing on brave://settings with tor window redirect to normal window?

Just re-checked, no, it does not redirects to a normal window. Probably I had confused between windows in previous comment, sorry for that.

@AlexeyBarabash
Copy link
Contributor

@simonhong so the private window and the guest window are different things?

@simonhong
Copy link
Member Author

simonhong commented Feb 18, 2019 •
edited
Loading

@AlexeyBarabash Yes, they are different.
Private window uses off the record profile of normal profile(used in normal window). Both are pair.
Guest window has its own normal profile. However guest window uses off the record profile of owned normal profile.(this off the record profile is effective profile during the guest window life cycle)
Tor window is same as guest window.
So, private window has its counter part normal window. but tor/guest windows doesn't have counter part normal window.

Because of this reason, loading brave://settings in private window can be redirected but tor/guest can't.

@AlexeyBarabash
Copy link
Contributor

Thanks @simonhong , I hadn't know this and never opened guest window before.
Just verified PR works good with guest window.
Guest
image
Private
image

@simonhong
Copy link
Member Author

@AlexeyBarabash Thanks for checking! Tor and guest window both are based on guest profile.
So, they have same result :)

@simonhong simonhong merged commit 3d12185 into master Feb 18, 2019
@simonhong simonhong deleted the fix_crash_loading_sync_page_in_tor_widow branch February 18, 2019 09:33
@simonhong simonhong mentioned this pull request Feb 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexeyBarabash AlexeyBarabash AlexeyBarabash approved these changes

@darkdh darkdh Awaiting requested review from darkdh

Assignees

@simonhong simonhong

Labels
None yet
Projects
Sync - All platforms
Archived in project
Milestone
0.63.x - Release
Development

Successfully merging this pull request may close these issues.

Crash when trying to open brave://sync in tor window
2 participants
@simonhong @AlexeyBarabash